feat(Feature/GoogleAnalytics): Implemented Optout link feature for GA… #231
Conversation
Features/GoogleAnalytics/fields.json
Outdated
| "translatableOptions": [ | ||
| { | ||
| "name": "optOutConfirm", | ||
| "label": "Optout Confirm Text", |
Features/GoogleAnalytics/fields.json
Outdated
| "label": "Optout Confirm Text", | ||
| "type": "text", | ||
| "required": 1, | ||
| "default_value": "Are you sure you want to opted out of Google Analytics?" |
Features/GoogleAnalytics/fields.json
Outdated
| }, | ||
| { | ||
| "name": "optOutSuccess", | ||
| "label": "Optout Success Text", |
Features/GoogleAnalytics/fields.json
Outdated
| "label": "Optout Success Text", | ||
| "type": "text", | ||
| "required": 1, | ||
| "default_value": "You successfully opted out from Google Analytics for this website." |
There was a problem hiding this comment.
opted out of Google Analytics
| @@ -2,18 +2,43 @@ | |||
|
|
|||
| namespace Flynt\Features\GoogleAnalytics; | |||
|
|
|||
| define(__NAMESPACE__ . '\NS', __NAMESPACE__ . '\\'); | |||
There was a problem hiding this comment.
this is not used, unless I'm missing something?
| 'confirm' => (isset($googleAnalyticsOptionsTranslatable['optOutConfirm'])) ? $googleAnalyticsOptionsTranslatable['optOutConfirm'] : '', | ||
| 'success' => (isset($googleAnalyticsOptionsTranslatable['optOutSuccess'])) ? $googleAnalyticsOptionsTranslatable['optOutSuccess'] : '' | ||
| ]; | ||
| wp_localize_script('Flynt/Features/GoogleAnalytics', 'wpData', $data); |
There was a problem hiding this comment.
Calling this global variable wpData could lead to issues, especially since the same name is used in this feature here:
We should prefix this to make sure nothing can go wrong (for example flynt_feature_google_analytics). The same should be done in the feature mentioned above in a separate PR.
Features/GoogleAnalytics/script.js
Outdated
| setOptoutCookie() | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
I really don't like having two separate scripts (this file and the script.twig file) loaded in the same feature for no apparent reason. Either this functionality should be moved or the existing script should be extracted into this file. Defining and passing the same variables twice using two different approaches makes little sense.
Also it needs to be noted that the order in which these two scripts load is of importance. In this case it works fine, because this file is loaded first. But if that changed for some reason, it's not obvious that the opt out would not work anymore (since the ga() call would be triggered before the window variable is set).
There was a problem hiding this comment.
I think we should keep it seperated since the script.twig is really only for the Google Analytics script tag and doesn't really have own logic in there. Also the GoogleAnalytics.php passes down some anonymizeIP functionality into the script.twig view.
Features/GoogleAnalytics/script.js
Outdated
| import $ from 'jquery' | ||
| import 'file-loader?name=vendor/js-cookie.js!js-cookie/src/js.cookie' | ||
|
|
||
| const $gaOptoutLinks = $('.globalAction-optoutGa') |
There was a problem hiding this comment.
I feel like a class is too restrictive for this. What if I wanted to do the opt out programmatically instead? This approach would not allow for that. Also, further down you remove all opt out "links", not giving anyone the choice to maybe use a button for example that could be set to disabled instead.
Why not just have a function available here that can be called in a component and let the component handle the visual counterpart (click event, removing the element or toggling a class)?
package.json
Outdated
| @@ -78,7 +78,9 @@ | |||
| "postbump": "gulp replaceVersion && git add ." | |||
| } | |||
| }, | |||
| "dependencies": {}, | |||
| "dependencies": { | |||
| "js-cookie": "^2.1.4" | |||
There was a problem hiding this comment.
please move this dependency to devDependencies
| $data = [ | ||
| 'gaId' => $googleAnalyticsOptions['gaId'], | ||
| 'confirm' => (isset($googleAnalyticsOptionsTranslatable['optOutConfirm'])) ? $googleAnalyticsOptionsTranslatable['optOutConfirm'] : '', | ||
| 'success' => (isset($googleAnalyticsOptionsTranslatable['optOutSuccess'])) ? $googleAnalyticsOptionsTranslatable['optOutSuccess'] : '' |
There was a problem hiding this comment.
- these variables are quite wordy and could be shortened
- instead of doing an isset and passing an empty string here, you should just pass the value and check it in the script if it's false before you run the alert (and maybe not run an empty alert in that case, which is weird). Same goes for the confirm box of course.
…ting helper functions for use i
|
@Qakulukiam this PR is also updated. If you still want (me) to refactor, just add notes on what I could do to improve this feature. |
|
Could you tell me how this feature addition would be used? (And please also add it to the readme.) Because as far as I can see, it is not going to work since the window variable is only set after the confirm popup. Then a cookie is set, but that cookie doesn't do anything. For more info on what actually disables Google Analytics, check this link. Please also test this with a real GA account after your adjustments, just to double-check. |
|
@Qakulukiam you're right. definitely needs more testing to see where to actually modify the GA optout value. |
… optout links